Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track callsite for observers & hooks #15607

Merged

Conversation

SpecificProtagonist
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist commented Oct 3, 2024

Objective

Fixes #14708

Also fixes some commands not updating tracked location.

Solution

ObserverTrigger has a new caller field with the track_change_detection feature;
hooks take an additional caller parameter (which is Some(…) or None depending on the feature).

Testing

See the new tests in src/observer/mod.rs


Showcase

Observers now know from where they were triggered (if track_change_detection is enabled):

world.observe(move |trigger: Trigger<OnAdd, Foo>| {
    println!("Added Foo from {}", trigger.caller());
});

Migration

  • hooks now take an additional Option<&'static Location> argument

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 3, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this strategy, but I really want to see benchmarks before we remove the feature flag on BundleInserter.

@alice-i-cecile alice-i-cecile added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Oct 3, 2024
@SpecificProtagonist SpecificProtagonist changed the title Track callsite for observers Track callsite for observers & hooks Oct 3, 2024
@SpecificProtagonist
Copy link
Contributor Author

SpecificProtagonist commented Oct 3, 2024

I tried running the various add_remove & insert_simple benches, but I need to figure out how to turn of frequency scaling for my laptop first (can't get consistent enough results).

Should this ends up having to sit behind a feature flag, would this be a new feature flag or the (possibly renamed) track_change_detection? And should hooks then take an Option<&Location>?

@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Oct 13, 2024
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
@NthTensor NthTensor mentioned this pull request Dec 11, 2024
6 tasks
@NthTensor
Copy link
Contributor

Personally, I'd rename track_change_detection and gate all tracking behind it.

For hooks, that's a bit harder. Ideally an optional Option<&Location> parameter, so that people don't have to litter their code with cfg attributes.

@SpecificProtagonist
Copy link
Contributor Author

Personally, I'd rename track_change_detection and gate all tracking behind it.

For hooks, that's a bit harder. Ideally an optional Option<&Location> parameter, so that people don't have to litter their code with cfg attributes.

I think that would be the best, will do. Can you add the rename to your meta issue?

@NthTensor NthTensor self-requested a review December 12, 2024 16:54
@NthTensor NthTensor added this to the 0.16 milestone Dec 12, 2024
ci

fix new hooks in dependant crates

ci
@ItsDoot ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 17, 2024
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 25, 2024
Copy link
Contributor

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

Copy link
Contributor

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@SpecificProtagonist SpecificProtagonist added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 10, 2025
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work. Trying to tie failures directly to user actions rather than via engine internals in a stack trace is definitely worth pursuing. I do have a couple suggestions that should be tackled in a follow-up PR to address some of the ergonomics issues we're starting to collect with track_caller. But as stated, these are follow-up concerns.

//
// `on_add` will trigger when a component is inserted onto an entity without it
.on_add(|mut world, entity, component_id| {
.on_add(|mut world, entity, component_id, caller| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up PR: The signature for a hook is getting pretty unwieldy. The component_id was already an infrequently used parameter (IMO), and I imagine users ignoring caller (even if they shouldn't!). It might be worth refactoring entity, component_id, and caller into a Context struct. I've omitted world from that context since it's the only one out of these 4 that a user will mutate, so keeping it separate will avoid some partial-mutable-borow pains.

Comment on lines +1069 to +1070
#[cfg(feature = "track_location")]
caller,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up PR: I think this pattern is pretty annoying to deal with. It's verbose and off-putting (IMO). I think we should investigate if obfuscating this behind a Context type which internally contains this feature gate is easier to use without sacrificing performance.

As much as I'm a no_std advocate, another alternative could be to use thread-local storage to manage this information without explicitly passing it as a function argument.

@NthTensor
Copy link
Contributor

I do have a couple suggestions that should be tackled in a follow-up PR to address some of the ergonomics issues we're starting to collect with track_caller. But as stated, these are follow-up concerns.

I'll add these to #16775 when I get a sec.

Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
entity,
component_id,
#[cfg(feature = "track_location")]
Some(caller),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use MaybeLocation instead of Option<&Location>? That would make it clear that the value is always present when the feature flag is enabled, and is never present when it's disabled.

That could be discussed in a follow-up, though! Really, I'd like to use something like that everywhere so that we don't need cfgs everywhere and I don't get CI failures because I forgot to build with the feature toggled. But I think that's more controversial so we shouldn't block this PR on that discussion.

Copy link
Contributor Author

@SpecificProtagonist SpecificProtagonist Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to coalesce the entity, component id and caller arguments into a single struct in a followup PR as suggested by bushRAT, which will have the same effect :)

crates/bevy_ecs/src/hierarchy.rs Outdated Show resolved Hide resolved
@BenjaminBrienen BenjaminBrienen added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jan 22, 2025
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jan 22, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 22, 2025
Merged via the queue into bevyengine:main with commit f32a6fb Jan 22, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track callsites of where observers are initially triggered.
7 participants